Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow config file to override command line parameters #761

Merged

Conversation

kennethpalmer
Copy link
Contributor

@kennethpalmer kennethpalmer commented Dec 20, 2023

🗣 Description

Configuration file parameters can now be overridden selectively from the command line. This allows re-use of config files. This is invoked with the -ConfigFilePath option and supplying other command line options

💭 Motivation and context

Currently if a configuration file is re-used and any changes are needed, an entire config file needs to be re-defined.
This will reduce the maintenance of config files.

Related to this, issue #345 poor defaults has been resolved by the current config module implementation

This change was implemented by 3 changes to the Orchestrator code

  1. All of the command line options in the ReportGroup have been added to the Configuration group so they are visible when using of a config file.

  2. The ConfigFilePath is added as a mandatory option in the Configuration group which triggers that group. Otherwise the Report Group is triggered.

  3. If the Configuration group is active, the following additional processing is performed

  • The code expects authentication parameters ( if present ) to reside in $PSBoundParameters ( values passed in from the command line ). Therefore if an authentication is set in the config file and not present in the bound parameters already copy the value to the bound parameters. This allows any command line value to override the configuration file value
  • Next any $PSBound parameter that is present other that ConfigFilePath is copied to the $PSBoundParameters. Then if a respective $PSBoundParameter is set the option has either been passed on the command line or was an authentication related parameter, so update the config parameter used in the rest of the processing.
  1. ScubaConfig supports defaults, but two defects were resolved ( OPAPath and OutRegoFileName so it will now handle a config file with no parameters other than the description

The resolution of this issue should also resolve and close issue #345, which is no or poor defaults, since the ScubaConfig module code now provides these.

TODO: Parameter value defaults now have a duplicate definition in Invoke-SCuBA and ScubaConfig. This
has been filed as issue #767

TODO: Documentation is being updated as issue #344

🧪 Testing

The original submission was done with a manual testing process. Since that time I have developed a Pester test as a new module in the Orchestrator unit tests. There is already a unit test for the config tests and since these changes affect the orchestrator it was more appropriate to add them here. The Manual test is documented at the end of this section

Pester Test
A new pester test InvokeScubsConfigTests.ps1 has been added. The test invokes Invoke-SCuBA as follows ( using a hash for the arguments ).

Invoke-SCuBA  -M365Environment gcc -ConfigFilePath < full path to orchestrator_config_test.yaml file>

The orchestrator_config_test.yaml. file specifies the M365environment as commercial, but the command line overrides this to gcc. The resulting configuration ( apart from ConfigFIlePath ) is stored in the Configuration value of the ScubaConfig singleton class. The value is cloned to a separate instances, the ScubaConfig module is reloaded from the config file and the two values are compared.

For orchestrator_config_test.yaml has dummy authentication parameters in it ( no real values in this commit ).

Comparison is done invoking the newly written PsTestUtils.psm1 Compare-Hashes function. which returns true if the two configuration hash table instances compare .

On the first test, the compare should fail, indicating that the configuration was indeed modified and overwritten by the command line value

The second test is run like the first except the the reference hash table is read from the config file and the the M365Environment value is modifies to match the command line value. The compare as run now should pass

Manual testing process:
Prior to the development of the pester test above the following manual test was done.

1). Run
Invoke-SCuba -ConfigFilePath config_test.yaml
with following settings in config_test.yaml
( Authentication parameters are masked in the example below )

Description: YAML Configuration file for unit test in SCuBA

ProductNames:
- teams
M365Environment: commercial
OPAPath: .
LogIn: true
DisconnectOnExit: false
OutPath: .
OutFolderName: M365BaselineConformance
OutProviderFileName: ProviderSettingsExport
OutRegoFileName: TestResults
OutReportName: BaselineReports
Organization:  XXXX
AppID: XXXX
CertificateThumbprint: XXXX
  1. Run with overrrides ( example command shown )
    Invoke-SCuba -ProductNames aad -ConfigFilePath config_test.yaml

  2. Rerun with authentication parameter overrides

  3. Run with degenerate config file ( description only ) to verify defaults operation

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR. Document Support for Config Files #344
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@kennethpalmer kennethpalmer linked an issue Dec 20, 2023 that may be closed by this pull request
Copy link
Contributor

PSScriptAnalyzer results:

Errors: [0], Warnings: [0], Information: [1]


RuleName   : PSAvoidTrailingWhitespace
Severity   : Information
ScriptName : Orchestrator.psm1
Line       : 294
Message    : Line has trailing whitespace

@kennethpalmer kennethpalmer force-pushed the 158-allow-config-file-to-override-command-line-parameters branch 2 times, most recently from 5951a7a to f329526 Compare December 20, 2023 17:29
@schrolla schrolla changed the base branch from main to flipper December 21, 2023 21:55
@kennethpalmer kennethpalmer force-pushed the 158-allow-config-file-to-override-command-line-parameters branch 2 times, most recently from 4557dc8 to 1ec2e5f Compare December 22, 2023 15:28
@kennethpalmer kennethpalmer requested review from adhilto and crutchfield and removed request for adhilto December 22, 2023 16:21
@kennethpalmer kennethpalmer marked this pull request as ready for review December 22, 2023 16:22
Copy link
Collaborator

@schrolla schrolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick run using the testing indicated shows it works as advertised to allow the command line to override config file parameters. However, the linked issue referenced (#345) is not the one actually being addressed here (#158). That's indicated by the branch itself which is associated with #158 and the actual implementation which allows cmd line parameters to override the config file. It does not address #345 which would allow a minimal config file. See example run below where config file example was used, but the OutRegoFileName parameter was removed from the file. The expected behavior is for the run to succeed using the default value.

Right now, the text of the PR indicates it fixes #158 while referencing #345, but describes it as fixing what is actually #158. Either the PR should be updated to address the original issue #158 and text of the PR updated accordingly or descoped to reflect that it only addresses #345 and text updated to remove references to resolving poor(no) config file defaults.

Screenshot 2023-12-22 at 11 19 26 AM

@schrolla
Copy link
Collaborator

A quick run using the testing indicated shows it works as advertised to allow the command line to override config file parameters. However, the linked issue referenced (#345) is not the one actually being addressed here (#158). That's indicated by the branch itself which is associated with #158 and the actual implementation which allows cmd line parameters to override the config file. It does not address #345 which would allow a minimal config file. See example run below where config file example was used, but the OutRegoFileName parameter was removed from the file. The expected behavior is for the run to succeed using the default value.

Right now, the text of the PR indicates it fixes #158 while referencing #345, but describes it as fixing what is actually #158. Either the PR should be updated to address the original issue #158 and text of the PR updated accordingly or descoped to reflect that it only addresses #345 and text updated to remove references to resolving poor(no) config file defaults.

Screenshot 2023-12-22 at 11 19 26 AM

Latest commits address the issue in the screenshot and result in a successful run both with several "default" parameters not present or with an empty config which runs as if executed with no parameters (as expected).

Copy link
Collaborator

@schrolla schrolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running with several combinations of config and command line works as intended. Missing parameters not in config file use defaults as intended. Unit tests passing.

Resolve minor linting whitespace issue, but non-blocking. Approved.

PowerShell/ScubaGear/Modules/Orchestrator.psm1 Outdated Show resolved Hide resolved
@james-garriss
Copy link
Collaborator

Might be good to edit the title so that it doesn't include the number 158. See other PR names for examples.

@kennethpalmer kennethpalmer force-pushed the 158-allow-config-file-to-override-command-line-parameters branch from 4a23e63 to 1319fc3 Compare December 29, 2023 16:47
@kennethpalmer
Copy link
Contributor Author

Some changes made in the pester test to insure that there was not a false pass. However a limitation has been discovered. I had written the PsTestUtils.ps1 module to provide for comparing 2 hashtables. However as written, it will not successfully compare first level hashtable values that are not scalars. ProductNames and AnObject are examples of these, so they are added to an exclude list in the comparison utility. This is a workaround that does not invalidate the test ( since I override the M365Environment ) but it is currently not a general use utility

Copy link
Contributor

@crutchfield crutchfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@kennethpalmer kennethpalmer changed the title 158 allow config file to override command line parameters Allow config file to override command line parameters Jan 2, 2024
@kennethpalmer kennethpalmer force-pushed the 158-allow-config-file-to-override-command-line-parameters branch 2 times, most recently from cf5dc6a to 6e27f19 Compare January 4, 2024 02:15
Copy link
Contributor

@crutchfield crutchfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

Testing/Unit/PowerShell/PsTestUtils.psm1 Outdated Show resolved Hide resolved
Testing/Unit/PowerShell/PsTestUtils.psm1 Outdated Show resolved Hide resolved
Testing/Unit/PowerShell/PsTestUtils.psm1 Outdated Show resolved Hide resolved
Testing/Unit/PowerShell/PsTestUtils.psm1 Outdated Show resolved Hide resolved
@kennethpalmer kennethpalmer self-assigned this Jan 4, 2024
@kennethpalmer kennethpalmer added this to the Flipper - Jan 2024 milestone Jan 4, 2024
@kennethpalmer kennethpalmer force-pushed the 158-allow-config-file-to-override-command-line-parameters branch from 259c3b0 to 91a079e Compare January 7, 2024 14:21
@crutchfield crutchfield force-pushed the 158-allow-config-file-to-override-command-line-parameters branch from 15dd2e5 to c5d7f0b Compare January 24, 2024 16:04
@crutchfield crutchfield requested a review from schrolla January 24, 2024 16:15
Copy link
Collaborator

@schrolla schrolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crutchfield Approving, but see last comment on README ToC for consideration prior to merge.

@crutchfield crutchfield force-pushed the 158-allow-config-file-to-override-command-line-parameters branch from c5d7f0b to 391628d Compare January 24, 2024 17:04
kennethpalmer and others added 18 commits January 25, 2024 10:58
Orchestrator Scuba config test simplification
OverrideTest local function is simplified so seperate module is no longer called.

Co-authored-by: Richard Crutchfield <crutchfield@users.noreply.github.com>
…tor ScubaConfig tests

This will enable this external module to run with top level Pester-Test
Reference:  https://www.logitblog.com/mastering-mocking-with-pester-commandnotfoundexception/
…tor ScubaConfig tests

This will enable this external module to run with top level Pester-Test
Reference:  https://www.logitblog.com/mastering-mocking-with-pester-commandnotfoundexception/
Remove unnecessary whitespace
Fix comment misspellings
Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
@schrolla schrolla force-pushed the 158-allow-config-file-to-override-command-line-parameters branch from 391628d to 45f133f Compare January 25, 2024 16:58
@crutchfield
Copy link
Contributor

@nanda-katikaneni Ready to merge

@nanda-katikaneni nanda-katikaneni merged commit 60da4bd into main Jan 25, 2024
4 checks passed
@nanda-katikaneni nanda-katikaneni deleted the 158-allow-config-file-to-override-command-line-parameters branch January 25, 2024 19:10
nanda-katikaneni pushed a commit that referenced this pull request Jan 26, 2024
* WIP

* WIP

* Fix paths

* Add UT

* Fix linter issue

* WIP

* WIP

* Add UT

* Add suppress for unused parameter

* Fix ErrorAction

* Remove blank

* WIP

* WIP

* WIP

* WIP

* Fix URL to baselines

* Fix link to baseline documents

* disable PnP update banner (#849)

* Allow config file to override command line parameters (#761)

* Apply suggestions from code review
Orchestrator Scuba config test simplification
OverrideTest local function is simplified so seperate module is no longer called.

Co-authored-by: Richard Crutchfield <crutchfield@users.noreply.github.com>

* Eliminate superfulous content in orchestrator_config_test.yaml

* Fix lint errors in yaml file and add mutiple product test case in
overrides

* Add pester test for credentails passed in as arguments that are not in
config file.

* Added reset to ScubaConfig invocation to insure it is reloaded from config

* Added missing change to prior commit

* Fix comparsion with $null statements

* Add local funcion declaration for Disconnect-SCuBATenant in Orchestrator ScubaConfig tests
This will enable this external module to run with top level Pester-Test
Reference:  https://www.logitblog.com/mastering-mocking-with-pester-commandnotfoundexception/

* Back out experimental change left on last commmit

* Add local funcion declaration for Disconnect-SCuBATenant in Orchestrator ScubaConfig tests
This will enable this external module to run with top level Pester-Test
Reference:  https://www.logitblog.com/mastering-mocking-with-pester-commandnotfoundexception/

* Back out experimental change left on last commmit

* Fix test typo (Shoud -> Should)
Remove unnecessary whitespace
Fix comment misspellings

* Update UT

* Fix trailing space

* Force import module

* Debug scope issue

* Fix TOC to ignore unwanted headers

* Update PowerShell/ScubaGear/Modules/Orchestrator.psm1

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

---------

Co-authored-by: Richard Crutchfield <crutchfield@users.noreply.github.com>
Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
Co-authored-by: Richard Crutchfield <richard.m.crutchfield@gmail.com>

* Fix Group Anchor URL

* Revert automatic changes to Markdown

* Update PowerShell/ScubaGear/Testing/Unit/PowerShell/Support/Copy-ScubaBaselineDocument.Tests.ps1

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

* Update PowerShell/ScubaGear/Testing/Unit/PowerShell/RunPesterTests.ps1

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

* Add newline at EOF

* WIP

* WIP

* Fix paths

* Add UT

* Fix linter issue

* WIP

* WIP

* Add UT

* Add suppress for unused parameter

* Fix ErrorAction

* Remove blank

* WIP

* WIP

* WIP

* WIP

* Fix URL to baselines

* Fix link to baseline documents

* Fix Group Anchor URL

* Revert automatic changes to Markdown

* Update PowerShell/ScubaGear/Testing/Unit/PowerShell/Support/Copy-ScubaBaselineDocument.Tests.ps1

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

* Update PowerShell/ScubaGear/Testing/Unit/PowerShell/RunPesterTests.ps1

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

* Add newline at EOF

* rebase

* Add readme reference to baselines

* Update reference to individual documents

* Add link to baseline folder

---------

Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
Co-authored-by: Kenneth S Palmer <kpalmer@mitre.org>
Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality enhancement This issue or pull request will add new or improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow config file to override command line parameters
6 participants